ci(test): run the test job on windows too - #272

The seventeen are dealt with and the Windows job is green: 137 files, 1609 passing, 28 skipped, coverage at 91.94 statements, 89.07 branches, 91.41 functions and 93.47 lines, every floor clear. Linux is unchanged at 1635 passing and 2 skipped.

Sixteen of the seventeen were the tests being wrong about Windows. One was not, and it was hiding behind twelve of them.

The bug

Every EXECUTE_GAME fixture in gameHandlers.test.ts writes a game binary called Vintagestory, and on Windows buildGameLaunchPlan only ever looks for Vintagestory.exe. So the version folder held no game the launcher could see, every test got no-executable back, and nothing past that point ran at all. That is the whole story for twelve failures, the two that looked like they diverged on session adoption with mismatched uids included: with no launch there is no session write, so there was never anything to adopt, and the file the assertions read back still held whatever the fixture had put there. Naming the binary per platform in one place fixed the diagnosis, and it also let the Windows job get far enough to spawn the thing, which is where the real problem was waiting.

child_process.spawn reports ENOENT, EACCES, EAGAIN, EMFILE and ENFILE through an error event. Everything else it throws where it stands. Windows answers a file that is not a valid executable with UNKNOWN, which is none of those five, and both spawns in gameHandlers.ts were written for the event alone. The throw went straight past the promise and out through the handler, so EXECUTE_GAME rejected instead of resolving launch-failed, which is precisely the anti-pattern gameProcessOutcomeToResult was written to end, and LOOK_FOR_A_GAME_VERSION rejected instead of reporting no version found.

A player reaches this with a game version whose executable a stopped download truncated, or that an antivirus emptied in place. On Linux that is EACCES, the event fires, and they get the ordinary "couldn't run it" notice. On Windows they got the generic error the renderer shows for any exception, and none of the log lines the failure path writes. Both spawns now catch the throw and settle it the way the error event does. Two tests pin it, and they drive the throw through a spawn wrapper rather than through a real Windows failure, so they hold the contract on Linux too rather than only where the bug shows.

The verdicts

On the adoption pair

Since the brief singled those two out, the reading is worth stating plainly. Nothing in the adoption path branches on platform. sessionToAdopt compares playeruid with !==, which is right for an identifier the auth service issues, and there is no case folding to do there. The settings file is found by joining clientsettings.json onto a path that has already been through assertManagedPath, and path policy compares through comparablePath, which already knows about Windows. Both failures were the launch dying two steps earlier. I checked before renaming anything, and the reproduction backs it up: pointing the fixture helper at a name the launcher does not know fails all twelve on Linux with the identical messages, mismatched uids and all.

Odds and ends

One test in gameHandlers.test.ts was passing on Windows for the wrong reason. It makes the version folder unlistable with chmod 0o000 to cover the readdir failure arm, and since NTFS has no such bits the folder lists fine, comes back empty, and yields no-executable down a different path. It skips on Windows now with that written on it, which is why the skip count moved by seven rather than six.

Two rounds of CI. The first took seventeen down to nine, all nine being the spawn throw the fixtures had been hiding. The second was green.

You are right, and I checked the protection rule myself before touching anything. dev requires exactly these contexts today, with enforce_admins: true:

typecheck, lint, test, build (ubuntu-latest), build (windows-latest)

So the four-line diff was a trap. Once test carries strategy.matrix.os, GitHub reports test (ubuntu-latest) and test (windows-latest), and the bare test context is never produced again. Every open PR against dev would sit forever on a status nothing can report, this one included, with no admin override. The scar tissue is already visible in the required list: build (ubuntu-latest) and build (windows-latest) are in there because build got matrixed at some point and someone had to go edit the rule afterwards.

Took your option 2, since it is the one the PR can carry without a coordinated window. The matrix job is now test-matrix, and a gate job keeps the required name alive:

  test:
    needs: [test-matrix]
    if: always()
    runs-on: ubuntu-latest
    steps:
      - run: |
          echo "test-matrix result: ${{ needs.test-matrix.result }}"
          [ "${{ needs.test-matrix.result }}" = "success" ]

Both halves are load bearing, as you said. Without always() the gate is skipped when a leg fails, and protection treats a skipped required job as satisfied, which is the failure mode where a red matrix merges anyway. And the check has to be against success rather than something looser: for a matrixed dependency, needs.test-matrix.result is success only when every leg succeeded, and it is failure, cancelled or skipped in every other case. The string comparison is false for all three, so the step exits non-zero and the gate goes red. Cancellation and a skipped matrix are covered by the same comparison, not just an errored leg.

About the failing case, I want to be honest about how far I went. I did not actually break a leg to watch the gate turn red, because doing that means pushing a deliberately broken commit onto this branch and then reverting it, which leaves noise in the history you are reviewing. What I have instead is the argument above plus the shape of the expression: [ "$x" = "success" ] under the default bash shell exits 1 for any other value, and needs.<job>.result has no values beyond those four. The echo line is there so a future red gate says in its own log which state it saw, rather than making someone reconstruct it. If you want the empirical version before approving, say so and I will run it once on a throwaway branch.

The proof that matters is on this run:

test                          pass  4s
test-matrix (ubuntu-latest)   pass  1m42s
test-matrix (windows-latest)  pass  2m44s
typecheck                     pass
lint                          pass
build (ubuntu-latest)         pass
build (windows-latest)        pass
sonarcloud                    pass

A context literally named test is reported again and it is green, so all five required contexts still exist. sonarcloud is untouched: no needs:, its own test:coverage run, still ubuntu only.

Local gates on this commit: typecheck clean, lint:ci at 0 errors and the same 15 pre-existing warnings, format:check clean, test:coverage at 92.72 / 89.84 / 92.12 / 94.19, matching your numbers and above the floors.

Leaving your non-blocking points for a separate pass rather than folding them into this one. And if the maintainers would rather have option 1 later, editing the required contexts to the two matrix names, then the gate job becomes dead weight and can be deleted in that same change.

Landed your four non-blocking points here rather than in a follow-up, so the same file set gets one review instead of two.

spawnThrow.next. Reset in beforeEach alongside the rest of the per-test state. Nothing was passing for the wrong reason: both tests that raise the flag assert it was consumed, so the leak needs a test to fail before it reaches the spawn, and the damage then lands on whatever runs next. Cheap to close, and it is the same cross-test pollution the renderer harness work went after.

The settle / resolve asymmetry. timer now starts as undefined above settle, so the spawn catch settles exactly the way the error event does and clearTimeout ignores the handle that does not exist yet. The catch also passes stdout rather than a literal "", which is the same value at that point and removes the last difference between the two paths. One thing worth knowing if you touch it: prefer-const flags a bare let timer declared and assigned once, and const is what puts the TDZ back, so the explicit = undefined is load bearing rather than noise.

.gitattributes. * text=auto eol=lf, plus binary on *.zip, *.bin, *.lzma1 and *.lzma2. The fixture markers are not ceremony: I checked every file under tests/fixtures, and not-a-zip.bin contains no NUL byte, so text=auto classifies it as text on its own. It carries no CR today, which is why nothing has broken yet, but regenerate it with one and the byte-exact assertions start disagreeing with the file. Images are marked too. Nothing in the index is CRLF right now, so this renormalises no existing file and the diff is the one new file. I left the .replace(/\r\n/g, "\n") in accountLoginFlow.test.ts alone, since existing clones keep their CRLF working copies until someone refreshes them.

The commit message on 37e86de. Verified your reading before writing anything down: MainMenu.tsx catches the rejection and shows notifications.body.errorExecutingGame, and playOutcomeNotifications.ts maps launch-failed to that same key, so the player saw one message either way. useLookForAVersion.ts:34 really is bare, and a rejected invoke there skips setFolder, setVersionFound and addNotification together, which is the dialog doing nothing at all. I did not amend the commit. You cite it by hash in your review and the green run hangs off it, so rewriting it would cost more than the wrong sentence does. The correction is in the PR description instead.

Checks on this push, with the required context still produced:

test                          pass  4s
test-matrix (ubuntu-latest)   pass  1m30s
test-matrix (windows-latest)  pass  2m19s
typecheck  pass | lint  pass | build (ubuntu-latest)  pass | build (windows-latest)  pass
sonarcloud pass

Locally: typecheck clean, lint:ci at 0 errors and the same 15 warnings, format:check clean, test:coverage at 92.79 / 89.87 / 92.21 / 94.28, and gameHandlers.test.ts still 26 passing.